Skip to content

Fixed test name#128

Merged
themiurgo merged 2 commits intopython-visualization:masterfrom
ocefpaf:fix_tests
Jul 29, 2015
Merged

Fixed test name#128
themiurgo merged 2 commits intopython-visualization:masterfrom
ocefpaf:fix_tests

Conversation

@ocefpaf
Copy link
Copy Markdown
Member

@ocefpaf ocefpaf commented May 13, 2015

Not ready to merge. I will add more stuff tomorrow morning.

@ocefpaf ocefpaf force-pushed the fix_tests branch 3 times, most recently from 9cf07ee to 104cc3e Compare July 28, 2015 15:22
@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 28, 2015

OK. I finally pushed all the remaining changes I had in my branch. They are all minor changes, but it fixes a few errors.

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 28, 2015

@BibMartin or @themiurgo can either of you review and merge?

@themiurgo
Copy link
Copy Markdown
Contributor

Two questions @ocefpaf:

  • any particular reason why you changed the { ... } with dict( ... )? In general, there is no accepted convention for this, but people tend to prefer and use the former. While this is debatable for multiline dicts, it seems the first is more practical / legible for stuff that is supposed to be in one line:
county_df = pd.DataFrame({"FIPS_Code": county_codes}, dtype=str)
county_df = pd.DataFrame(dict(FIPS_Code=county_codes), dtype=str)

and even more in the case of empty dicts ({} vs dict()). The literal syntax is also more efficient (http://doughellmann.com/2012/11/12/the-performance-impact-of-using-dict-instead-of-in-cpython-2-7-2.html).

  • I also noticed you changed Folium to folium within docs. Was this intended? I would keep the name upperscore (as it is referenced in the the rest of the docs). If we want to change it, we need to do be consistent and change in all the docs through the whole codebase.

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 29, 2015

any particular reason why you changed the { ... } with dict( ... )?

Nope. I just like dict() better (my brain changes that automatically while I am reading the code).

is more practical / legible for stuff that is supposed to be in one line

True

even more in the case of empty dicts ({} vs dict()).

That is a case where I prefer dict() for clarity: like set(), tuple() and list() anyways... I am not teaching code here so this does not apply ;-)

The literal syntax is also more efficient

Sold!

I also noticed you changed Folium to folium within docs. Was this intended?

It was, but you are right. Lets keep it consistent with the rest. I do not want anyone to waste time changing this in the rest of the code.

themiurgo added a commit that referenced this pull request Jul 29, 2015
@themiurgo themiurgo merged commit 397d8ca into python-visualization:master Jul 29, 2015
@themiurgo
Copy link
Copy Markdown
Contributor

And it's a merge! 🎉 Thanks!

@ocefpaf ocefpaf deleted the fix_tests branch July 29, 2015 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants